Skip to content

Define engine-to-integrator error contract#43

Merged
anormang1992 merged 1 commit intomainfrom
feat/vre-error-handling
Mar 30, 2026
Merged

Define engine-to-integrator error contract#43
anormang1992 merged 1 commit intomainfrom
feat/vre-error-handling

Conversation

@anormang1992
Copy link
Copy Markdown
Owner

Summary

Closes #42.

  • New core/errors.py — typed exception hierarchy rooted at VREError. Integrators can catch at desired granularity (individual error type → VREError catch-all).
  • Fix silent overrideCyclicRelationshipError in _persist_reachability now does rollback + re-raise instead of silently converting to SKIPPED. The engine cleans up its in-memory mutation; the integrator decides recovery strategy.
  • Wrap repository boundary — all PrimitiveRepository methods catch Neo4jError and raise GraphError (reads) or PersistenceError (writes). Integrators never see raw Neo4j driver exceptions.
  • Wrap hydration — corrupted JSON in the database raises HydrationError instead of raw json.JSONDecodeError/KeyError.
  • Safe cardinality default — unknown cardinality strings default to None, which fires all policies. If we can't determine the cardinality, we can't justify skipping any policy.
  • CyclicRelationshipError re-parented from ValueError to GraphIntegrityError(VREError). Breaking change — clean break, no backward-compat shim.

Error hierarchy

VREError
├── GraphError                    (any graph backend failure)
│   └── PersistenceError          (write-specific: save, delete)
├── GraphIntegrityError
│   └── CyclicRelationshipError   (re-parented from ValueError)
├── HydrationError                (corrupt data on read)
├── ResolutionError               (concept/name resolution)
└── CandidateValidationError      (bad candidate fields in learning)

Test plan

  • All 194 existing tests pass (updated for new exception types)
  • Cycle detection tests expect CyclicRelationshipError to propagate (not SKIPPED)
  • Cardinality test updated: unknown string → all policies fire → BLOCK
  • CandidateValidationError replaces ValueError in all learning engine validation
  • Coverage at 74.53% (above 70% threshold)
Implementation plan

Issue #42: Engine-to-Integrator Error Contract

Context

The LearningEngine silently overrides integrator decisions when post-decision failures occur (engine.py:279-283). More broadly, VRE lacks a consistent error contract — many errors propagate as raw ValueError or untyped exceptions, making it hard for integrators to handle them programmatically.

Principle: VRE's responsibility is to roll back any in-memory mutations and re-raise errors with clear, typed exceptions. Integrators decide recovery strategy. VRE does not make recovery decisions on behalf of integrators.

Guard decorator: Unhandled exceptions propagating out of vre_guard is acceptable — the integrator wraps the decorated function call in their own error handling.


Part 1: Error Hierarchy — new src/vre/core/errors.py

Dedicated module for the exception hierarchy. Keeps models.py focused on domain models.

VREError (base for all VRE exceptions)
├── GraphError                          # any graph backend failure (reads + writes)
│   └── PersistenceError                # write-specific failures (save, delete)
├── GraphIntegrityError
│   └── CyclicRelationshipError         # re-parented from ValueError
├── HydrationError                      # JSON/data corruption on read
├── ResolutionError                     # concept/name resolution failures
└── CandidateValidationError            # invalid candidate fields in learning
  • CyclicRelationshipError moves from core/models.py to core/errors.py. Clean break — no re-export from models.py.
  • Re-parented from ValueError to GraphIntegrityError(VREError). Breaking change — acceptable.
  • All existing imports update to from vre.core.errors import CyclicRelationshipError (or from vre import CyclicRelationshipError).

Part 2: Fix the Silent Override

File: src/vre/learning/engine.py:279-283

# Before — silent override:
try:
    self._repo.save_primitive(source)
except CyclicRelationshipError:
    depth_obj.relata.remove(new_relatum)
    return CandidateDecision.SKIPPED

# After — rollback + re-raise:
try:
    self._repo.save_primitive(source)
except CyclicRelationshipError:
    depth_obj.relata.remove(new_relatum)  # engine's cleanup responsibility
    raise                                  # integrator's decision

Exception propagates through learn_atlearn_all → integrator. The with callback: context manager ensures __exit__ fires for cleanup.


Part 3: Wrap Critical Sections

3a. Repository boundary — Neo4j (src/vre/core/graph.py)

Wrap all public methods so integrators never see raw Neo4j exceptions. Use GraphError for reads, PersistenceError for writes:

Method Exception
save_primitive() PersistenceError (let CyclicRelationshipError + provenance ValueError pass through)
delete_primitive() PersistenceError
find_by_id() GraphError
find_by_name() GraphError
list_names() GraphError
resolve_subgraph() GraphError
ensure_constraints() GraphError

3b. Hydration (src/vre/core/graph.py)

Wrap _hydrate_primitive with HydrationError for JSON/data corruption.

3c. Concept resolution (src/vre/core/grounding/resolver.py)

Replace RuntimeError with ResolutionError for missing spaCy model.

3d. Learning engine validation (src/vre/learning/engine.py)

Replace raw ValueError with CandidateValidationError in all _persist_* methods, _resolve_name_to_id, and learn_at.

3e. Policy callback resolution (src/vre/core/policy/models.py)

Wrap resolve_callback() with VREError for ImportError/AttributeError.

3f. Cardinality — safe default on bad input

Default to None on unknown cardinality. Update the gate to fire all policies when cardinality is unknown — if we can't determine cardinality, we can't justify skipping any policy.

🤖 Generated with Claude Code

@anormang1992 anormang1992 force-pushed the feat/vre-error-handling branch 2 times, most recently from 000bc8d to 6ea6544 Compare March 27, 2026 23:26
@anormang1992 anormang1992 requested a review from Copilot March 28, 2026 05:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Defines a typed, integrator-facing error contract across the VRE stack, ensuring engine/repository/grounding layers re-raise consistent VREError subclasses (instead of raw/untyped exceptions) and avoid silently overriding integrator decisions.

Changes:

  • Introduces vre.core.errors exception hierarchy and re-exports error types from vre.core and top-level vre.
  • Updates learning engine validation + cycle handling to raise typed exceptions and propagate CyclicRelationshipError (with in-memory rollback).
  • Wraps Neo4j repository operations and hydration to prevent raw driver/JSON exceptions from leaking to integrators; updates policy cardinality handling to “safe default” behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/vre/core/errors.py Adds new VRE exception hierarchy.
src/vre/core/__init__.py Re-exports new error types from vre.core.
src/vre/__init__.py Re-exports error types from top-level package; adjusts cardinality parsing to safe-default behavior.
src/vre/core/graph.py Wraps Neo4jError at repository boundary and wraps hydration failures with HydrationError.
src/vre/core/grounding/resolver.py Uses ResolutionError when spaCy model is missing.
src/vre/learning/engine.py Replaces ValueError with CandidateValidationError; propagates CyclicRelationshipError after rollback.
src/vre/core/policy/models.py Wraps policy callback dotted-path resolution failures as VREError.
src/vre/core/policy/gate.py Allows cardinality=None to mean “unknown → do not skip any policies.”
src/vre/core/models.py Removes CyclicRelationshipError from models (moved to errors module).
tests/vre/test_learning.py Updates tests for typed learning validation errors and cycle propagation behavior.
tests/vre/test_vre.py Updates tests for unknown-cardinality safe-default policy behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Establish a typed exception hierarchy and consistent error contract so
integrators never see raw ValueError/Neo4j exceptions. VRE rolls back
in-memory mutations and re-raises — integrators decide recovery strategy.

Key changes:
- New core/errors.py with VREError hierarchy (GraphError, PersistenceError,
  GraphIntegrityError, CyclicRelationshipError, HydrationError,
  ResolutionError, CandidateValidationError)
- Fix silent override: CyclicRelationshipError in _persist_reachability
  now does rollback + re-raise instead of silently returning SKIPPED
- Wrap all PrimitiveRepository methods with GraphError/PersistenceError
- Wrap hydration with HydrationError, resolver with ResolutionError
- Unknown cardinality defaults to None, firing all policies (safe default)
- CyclicRelationshipError re-parented from ValueError to GraphIntegrityError

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@anormang1992 anormang1992 force-pushed the feat/vre-error-handling branch from 6ea6544 to 2179d86 Compare March 30, 2026 13:43
@anormang1992 anormang1992 merged commit 0294949 into main Mar 30, 2026
2 checks passed
@anormang1992 anormang1992 deleted the feat/vre-error-handling branch March 30, 2026 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define engine-to-integrator error contract for callbacks

2 participants